feat(lint): require security suppressions to be reviewed in an allowlist#2343
Conversation
Every `# noqa: S<n>` (flake8-bandit) and `# nosec` (bandit) comment silences a security finding. The hub tar-slip (CWE-22) shipped because a `# noqa: S202 - hub artifacts are trusted` hid an unvalidated extractall from both bandit and the weekly audit, which read the suppression as an accepted call. This gate makes every such suppression an explicit, justified entry in .security-suppressions.json; a suppression not listed there fails lint, forcing a human to review it in the PR. It does not judge whether a suppression is correct — it guarantees a human signed off. Seeded with the 4 existing suppressions. Runs via `python util/lint.py --all` (or `--security`); lint.yml now also triggers on hub/** and the allowlist. Backtest confirms the gate flags the tar-slip's S202 suppression.
github-actions
Bot
added
devops
|
Verdict: Approve ✅ This adds a lint gate that forces every Two small gaps worth a follow-up (not blockers): a blanket 🔍 Technical details🟢 Minor / nits1. Duplicate (and renumber imports→ 2. Blanket 3. String/docstring literals are scanned too ( Strengths
|
…review-gate # Conflicts: # .security-suppressions.json # tests/unit/test_check_security_gates.py # util/check_security_gates.py # util/lint.py
… tooling (amd#2346) ## Why this matters The weekly audit's single general "security lens" missed the hub tar-slip (CWE-22, amd#2342): it sampled a large repo in one pass and read a `# noqa: S202 - hub artifacts are trusted` comment as a settled decision. This adds a **dedicated** security audit that fixes both failure modes and scores findings the way we actually triage them. - **Deterministic layer** — semgrep security rulesets over the whole tree, native SARIF → private code-scanning tab. Maintained rules, exhaustive on known sinks. - **Reasoning layer** — Claude over an explicit deterministic worklist (so nothing is sampled away), in three lenses: **sink-taint** (the CWE-22 class), **authz** (the CWE-284 read/write gate-asymmetry class), and **suppression-review** (every `# noqa: S*`/`# nosec` re-verified from scratch — never trusted, which is the exact failure that hid the tar-slip). - **CVSS 4.0 done right** — `util/cvss4.py` scores a *reviewed vector* via the `cvss` lib (validated against the FIRST v4 calculator, incl. the 6.0 tar-slip vector). Scores are arithmetic on a vector, never an LLM's guessed number — a real triage on the ticket guessed 7.3 for a vector that actually scores 8.7. - **Private by design** — findings go to the repo's Security → Code scanning tab (CVSS-derived `security-severity`, stable fingerprints), never a public issue. `SECURITY.md` adds a private reporting channel. ## Validation against the two reported vulnerabilities - The deterministic worklist surfaces **both** locations: the `read_file`/`write_file` + `is_path_allowed`/`validate_write` asymmetry (CWE-284) is handed to the authz lens, and the `extractall` sink (CWE-22) to sink-taint. - The suppression gate (PR amd#2343) already backtests catching the tar-slip's `# noqa: S202`. - The LLM layer is validated post-merge by dispatching this workflow with `target_ref` set to a known-vuln commit (pre-amd#2342 for CWE-22, pre-amd#2344 for CWE-284) and confirming alerts appear. ## Test plan - [x] `pytest tests/unit/test_cvss4.py tests/unit/test_findings_to_sarif.py` — 13 pass (FIRST-anchored CVSS incl. 6.0/8.7, SARIF shape, stable fingerprints) - [x] `cvss` lib matches the FIRST v4 calculator on 4 anchors - [x] workflow YAML parses; deterministic worklist confirmed to surface both CWE locations - [ ] Post-merge: `workflow_dispatch` at a pre-fix ref; confirm code-scanning alerts for both CWEs - [ ] Precondition: confirm code scanning is enabled + org allows third-party SARIF on `amd/gaia` ## Follow-ups (not in this PR) - Strip the now-redundant `security` dimension from `claude-weekly-audit.yml` + its skill/plan docs. - Enable the bandit-HIGH blocking gate after the 18 pre-existing HIGH findings are fixed. --------- Co-authored-by: Ovtcharov <kovtchar@amd.com>
3 participants
Why this matters
The hub tar-slip (CWE-22, #2342) shipped even though bandit flagged it — because a
# noqa: S202 - hub artifacts are trustedsilenced the warning, and both bandit and the weekly audit read that suppression as a settled, accepted decision. Any# noqa: S<n>/# nosecis a place where a human waved off a security finding with no required review.This adds a lint gate that makes every security suppression an explicit, justified entry in
.security-suppressions.json. A suppression in the code that isn't listed there failspython util/lint.py, forcing the author to add a justification the reviewer sees in the PR diff. It doesn't judge whether a suppression is correct — it guarantees a human signed off on each one, which is exactly the step that was missing for the tar-slip.Only real bandit security codes count —
SLF001and other non-securityS-prefixed codes are ignored. Entries key on(path, rule), never a line number, so the allowlist survives edits instead of re-firing on every line shift.Threads
check_suppressions, lint step 7) — the new gate this PR adds; seeded with the 6 suppressions already in the tree (S603×3,B104,B602×2).main— after mergingmain, both gates live inutil/check_security_gates.pyand both run fromutil/lint.py. This PR does not change the HIGH gate; it only adds the suppression-review gate next to it.Test plan
pytest tests/unit/test_check_security_gates.py— 30 pass, incl. a backtest that the gate flags the tar-slip's# noqa: S202python util/check_security_gates.py— suppression gate reports all 6 suppressions reviewedblack/isortclean on the changed filesmainresolved (both gates unified in one module; lint steps renumbered to 11)